Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(NODE-3727): add overloads for BulkOperationBase's execute function #3018

Merged
merged 3 commits into from
Nov 2, 2021
Merged

fix(NODE-3727): add overloads for BulkOperationBase's execute function #3018

merged 3 commits into from
Nov 2, 2021

Conversation

skrtheboss
Copy link
Contributor

@skrtheboss skrtheboss commented Oct 28, 2021 •
edited
Loading

Description

Add overloads for BulkOperationBase's execute function.

What is changing?

This adds overloads to the execute function, to ensure void is returned if a callback is provided,
and a promise is returned if no callback is specified.

Is there new documentation needed for these changes? No.

What is the motivation for this change?

Missing overloads.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@dariakp dariakp added the External Submission PR submitted from outside the team label Oct 28, 2021
@dariakp
Copy link
Contributor

dariakp commented Oct 28, 2021

@skrtheboss Thank you for submitting this suggestion. Can you please clarify what use case you are trying to support? We try to ensure that we add any missing use cases to our type test coverage so that we can avoid regressions.

@skrtheboss
fix: add overloads for BulkOperationBase's execute function
5c4d381 
This adds overloads to the execute function, to ensure void is returned if a callback is provided, and a promise is returned if no callback is specified.
@skrtheboss
Copy link
Contributor Author

HI @dariakp .
Thanks for the hint. I have updated the commit message and have added type tests.
Let me know if i implemented it correctly.

@skrtheboss skrtheboss changed the title Add overloads for BulkOperationBase's execute function fix: add overloads for BulkOperationBase's execute function Oct 29, 2021
@dariakp dariakp changed the title fix: add overloads for BulkOperationBase's execute function fix(NODE-3727): add overloads for BulkOperationBase's execute function Oct 29, 2021
dariakp
dariakp reviewed Oct 29, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dariakp
dariakp reviewed Oct 29, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding tests for this implementation, we'll fully review the PR soon. Meanwhile, I created this ticket for tracking: NODE-3727

@dariakp dariakp added tracked-in-jira Ticket filed in MongoDB's Jira system Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Oct 29, 2021
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 2, 2021
@dariakp dariakp requested a review from nbbeeken November 2, 2021 20:02
nbbeeken
nbbeeken approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dariakp
dariakp approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your submission!

@dariakp dariakp merged commit 1c204a0 into mongodb:main Nov 2, 2021
dariakp added a commit that referenced this pull request Nov 2, 2021
@skrtheboss @dariakp
fix(NODE-3727): add overloads for BulkOperationBase's execute function (
216d194 
#3018)

Co-authored-by: Daria Pardue <[email protected]>
@skrtheboss skrtheboss deleted the fix/bulk-execute-types branch November 3, 2021 07:17
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
@skrtheboss @dariakp @ljhaywar
fix(NODE-3727): add overloads for BulkOperationBase's execute function (
9d042a0 
#3018)

Co-authored-by: Daria Pardue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariakp dariakp dariakp approved these changes

@nbbeeken nbbeeken nbbeeken approved these changes

Assignees
No one assigned
Labels
External Submission PR submitted from outside the team Team Review Needs review from team tracked-in-jira Ticket filed in MongoDB's Jira system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skrtheboss @dariakp @nbbeeken